Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

feat: [WD-14882] Adjust Listen and Connect Inputs #890

Merged
merged 1 commit into from
Sep 18, 2024

Conversation

Kxiru
Copy link
Contributor

@Kxiru Kxiru commented Sep 12, 2024

Done

  • Split Listen and Connect values into 3 different inputs.
  • Disabled Connect Type is NAT is enabled.
  • Tested for inherited devices.

QA

  1. Run the LXD-UI:
  2. Perform the following QA steps:
    • Review the config page for adding Proxy devices to an instance or profile.

Screenshots

Updated:
image

@webteam-app
Copy link

@Kxiru
Copy link
Contributor Author

Kxiru commented Sep 12, 2024

It would be a nice to have to be able to have hover text on the disabled input for connect when NAT is enabled, however it does not take an onhover param. Any tips here, @edlerd ?

Also, @piperdeck , thanks in advance for the design review. The current code structure doesn't quite allow for the nicer look of having the input labels to the left of the input field. Even with the stacked attribute on the inputs, they end up looking indented and out of place. I tried exploring a few different ways to achieve your design but what not quite able. Perhaps some further light might be able to be shed on this by a reviewer...

Copy link
Collaborator

@edlerd edlerd left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Some ideas on the code simplification.
We need to be closer to the design. The labels should be in the left-hand column and listen/connect are not labels, and in italics.

src/components/forms/ProxyDeviceInput.tsx Outdated Show resolved Hide resolved
src/components/forms/ProxyDeviceInput.tsx Outdated Show resolved Hide resolved
src/components/forms/ProxyDeviceInput.tsx Outdated Show resolved Hide resolved
src/components/forms/ProxyDeviceInput.tsx Outdated Show resolved Hide resolved
src/components/forms/ProxyDeviceInput.tsx Outdated Show resolved Hide resolved
src/components/forms/ProxyDeviceForm.tsx Outdated Show resolved Hide resolved
@piperdeck
Copy link

I'm getting some strange behaviour where my mouse pointer is showing the pointing finger when I hover over the field label. This is different from when I hover over any other field label that isn't on this page:

CleanShot.2024-09-15.at.20.56.02.mp4

Also noticing that the address fields don't adapt to when Unix is selected as the connection type. Because unix sockets aren't like IP addresses, and they're of the form /path/to/socket.sock, the fields should change when I change the port type:

CleanShot 2024-09-15 at 20 58 33@2x

I'm not sure if this is what David is talking about in his previous comment, but when the user set NAT mode to true, the connection types for listen and connect should always be the same. Also, neither one can be Unix if NAT mode is enabled. But that behaviour isn't showing up at the moment:

CleanShot 2024-09-15 at 21 03 41@2x

These fields should probably be filled by their default values, rather than by "Select option":

CleanShot 2024-09-15 at 21 05 53@2x

I'm also noticing that the mode and UID/GID fields are missing when Unix is selected for the listen address. Did we opt not to include those?

@Kxiru Kxiru force-pushed the adjust-listen-and-connect-inputs branch 2 times, most recently from 442730c to c445b02 Compare September 16, 2024 13:54
@Kxiru
Copy link
Contributor Author

Kxiru commented Sep 16, 2024

@piperdeck , thanks for your review. A few comments :)

  • I have since been able to replicate your design! Apologies for the delay and thanks for creating it.
  • Enabling NAT now disables the type field in Connect and the Connect Type reflects the Listen type when it is changed.
  • Currently working on automatically matching the Connect Type to the Listen Type if/when they are indifferent before NAT is enabled.
  1. I was not aware of the Unix layout changing being a task or issue. I'll ask for more information on this.
  2. Regarding the mouse pointer, I find that the mous pointer becomes a pointer finger when hovering over the labels of any config in the GPU, Proxy, Other and Network tabs. I believe it is thus inkeeping with the existing style. I can look into this further if you would like/need?
  3. Regarding the default values, I was advised that Selecting an option would be a good way to move forward to avoid ... Missing some of the inputs? So users have to manually select each of their options.
  4. Are Mode and GID/UID inputs specific to Unix types? @edlerd , please could you advise on how to proceed here, as I believe we were removing the mode section but I'm not sure about the GID/UID.

Note to self: If NAT is enabled, neither Listen nor Connect can be UNIX.

@edlerd
Copy link
Collaborator

edlerd commented Sep 16, 2024

  • Regarding the mouse pointer, I find that the mous pointer becomes a pointer finger when hovering over the labels of any config in the GPU, Proxy, Other and Network tabs. I believe it is thus inkeeping with the existing style. I can look into this further if you would like/need?

I think this is default for HTML labels and should be all right from my point of view.

  • Regarding the default values, I was advised that Selecting an option would be a good way to move forward to avoid ... Missing some of the inputs? So users have to manually select each of their options.

I asked to remove the defaults earlier, because otherwise there is no way for a user to create a proxy device without those values being set. Now I am not so sure. Maybe we should preselect the default values that LXD will enforce?

Another thought: Should we remove the HAProxy protocol selector? I am not so sure how relevant it is, have a suspicion it is kind of a special use case thing we might want to hide from the average UI user.

  • Are Mode and GID/UID inputs specific to Unix types? @edlerd , please could you advise on how to proceed here, as I believe we were removing the mode section but I'm not sure about the GID/UID.

I think Mode, GID and UID are for very special use cases, maybe we can ignore them in the UI to keep the form simple?

@Kxiru
Copy link
Contributor Author

Kxiru commented Sep 16, 2024

  • Regarding the mouse pointer, I find that the mous pointer becomes a pointer finger when hovering over the labels of any config in the GPU, Proxy, Other and Network tabs. I believe it is thus inkeeping with the existing style. I can look into this further if you would like/need?

I think this is default for HTML labels and should be all right from my point of view.

  • Regarding the default values, I was advised that Selecting an option would be a good way to move forward to avoid ... Missing some of the inputs? So users have to manually select each of their options.

I asked to remove the defaults earlier, because otherwise there is no way for a user to create a proxy device without those values being set. Now I am not so sure. Maybe we should preselect the default values that LXD will enforce?

Another thought: Should we remove the HAProxy protocol selector? I am not so sure how relevant it is, have a suspicion it is kind of a special use case thing we might want to hide from the average UI user.

  • Are Mode and GID/UID inputs specific to Unix types? @edlerd , please could you advise on how to proceed here, as I believe we were removing the mode section but I'm not sure about the GID/UID.

I think Mode, GID and UID are for very special use cases, maybe we can ignore them in the UI to keep the form simple?

Noted. @piperdeck , how far do you agree with this?
I am happy to move forward in either case.

Copy link
Collaborator

@edlerd edlerd left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

One observation from QA

src/components/forms/ProxyDeviceForm.tsx Outdated Show resolved Hide resolved
@Kxiru Kxiru force-pushed the adjust-listen-and-connect-inputs branch from c445b02 to 399e45d Compare September 16, 2024 14:36
@piperdeck
Copy link

Regarding the default values, I was advised that Selecting an option would be a good way to move forward to avoid ... Missing some of the inputs? So users have to manually select each of their options.

I asked to remove the defaults earlier, because otherwise there is no way for a user to create a proxy device without those values being set. Now I am not so sure. Maybe we should preselect the default values that LXD will enforce?

My concern is that I want the user to actually know what the value of the default is. Like in the Bind field, with it set to Choose option, there's no intuitive way to know whether this configuration is going to bind to the host or the instance (for example). This is a tricky question. Am I understanding correctly that on the back end, there's a difference between implicitly keeping the default value and explicitly setting the default value?

In that case, is there any reason for use to ever leave that field implicit, rather than declaring it explicitly? I would lean toward that solution personally.

Regarding the mouse pointer, I find that the mous pointer becomes a pointer finger when hovering over the labels of any config in the GPU, Proxy, Other and Network tabs. I believe it is thus inkeeping with the existing style. I can look into this further if you would like/need?

I think this is default for HTML labels and should be all right from my point of view.

I'm ok with the behaviour in theory, it's just that it's not consistent with field labels on our other pages. It's setting off alarm bells for me that there's something different in the implementation that might need to be brought back in line. But even if all the code is ok, I think that this kind of consistency is important for maintaining a general feeling of quality

Regarding HAproxy and GUI/ID options, if they're really advanced options, I'm ok with reserving them for the YAML editor since we have it. We might have actually discussed this before, apologies if it's just my bad for forgetting :P

@Kxiru Kxiru force-pushed the adjust-listen-and-connect-inputs branch from 399e45d to 637d718 Compare September 16, 2024 15:51
@Kxiru Kxiru requested a review from edlerd September 16, 2024 15:55
@Kxiru Kxiru force-pushed the adjust-listen-and-connect-inputs branch from 637d718 to bde1f39 Compare September 16, 2024 15:59
src/components/ConfigurationRow.tsx Outdated Show resolved Hide resolved
src/components/forms/ProxyDeviceForm.tsx Outdated Show resolved Hide resolved
src/components/forms/ProxyDeviceForm.tsx Outdated Show resolved Hide resolved
src/components/forms/ProxyDeviceForm.tsx Outdated Show resolved Hide resolved
@Kxiru Kxiru requested a review from edlerd September 17, 2024 08:37
@Kxiru Kxiru force-pushed the adjust-listen-and-connect-inputs branch 2 times, most recently from 96495ae to 1729c79 Compare September 17, 2024 09:30
Copy link
Collaborator

@edlerd edlerd left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This looks very close to finished now, thanks for that. Some comments below.

src/components/forms/ProxyDeviceForm.tsx Outdated Show resolved Hide resolved
src/components/forms/ProxyDeviceForm.tsx Outdated Show resolved Hide resolved
src/util/proxyDevices.tsx Outdated Show resolved Hide resolved
src/util/proxyDevices.tsx Outdated Show resolved Hide resolved
src/util/proxyDevices.tsx Outdated Show resolved Hide resolved
src/util/proxyDevices.tsx Outdated Show resolved Hide resolved
@Kxiru Kxiru force-pushed the adjust-listen-and-connect-inputs branch from 1729c79 to 101763e Compare September 17, 2024 14:27
@Kxiru Kxiru requested a review from edlerd September 17, 2024 14:27
@Kxiru Kxiru force-pushed the adjust-listen-and-connect-inputs branch from 101763e to bcd8cd7 Compare September 17, 2024 14:45
Copy link
Collaborator

@edlerd edlerd left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for the adjustments, I found one probably last issue with changing the listen type when nat mode is enabled.

src/util/proxyDevices.tsx Outdated Show resolved Hide resolved
src/components/forms/ProxyDeviceForm.tsx Outdated Show resolved Hide resolved
src/util/proxyDevices.tsx Outdated Show resolved Hide resolved
@Kxiru Kxiru requested a review from edlerd September 17, 2024 21:00
@Kxiru Kxiru force-pushed the adjust-listen-and-connect-inputs branch from bcd8cd7 to 5947575 Compare September 17, 2024 21:00
Copy link
Collaborator

@edlerd edlerd left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

image

I think when switching to bind mode "instance", the NAT mode selector should be disabled with a title explaining "Only host-bound proxies can use NAT". If NAT mode is enabled, we should disable it. If NAT mode was not set, it shouldn't be overridden.

image
This is another restriction the form should represent. Though, it is much harder to represent. Maybe removing the HAProxy protocol selector is the easiest solution for it. As any other solution I can think of will be hard to understand for the user. And the field seems not so important to me.

src/util/proxyDevices.tsx Outdated Show resolved Hide resolved
src/util/proxyDevices.tsx Outdated Show resolved Hide resolved
@Kxiru
Copy link
Contributor Author

Kxiru commented Sep 18, 2024

@edlerd I'll remove the HAProxy input for now (and add this to the Spec), but isn't the solution just as simple as checking that NAT is disabled? Or is the problem moreso telling the User at the right time that certain configs aren't allowed?

@Kxiru Kxiru force-pushed the adjust-listen-and-connect-inputs branch from 5947575 to cf1cc76 Compare September 18, 2024 14:02
@Kxiru Kxiru requested a review from edlerd September 18, 2024 14:02
@Kxiru Kxiru force-pushed the adjust-listen-and-connect-inputs branch from cf1cc76 to d8b1cee Compare September 18, 2024 14:04
src/components/forms/ProxyDeviceForm.tsx Outdated Show resolved Hide resolved
src/components/forms/ProxyDeviceForm.tsx Show resolved Hide resolved
@edlerd
Copy link
Collaborator

edlerd commented Sep 18, 2024

isn't the solution just as simple as checking that NAT is disabled? Or is the problem moreso telling the User at the right time that certain configs aren't allowed?

There are many special cases here and handling them well is a can of worms. Probably best to avoid this complexity. Some examples:

  1. user already selected listen mode unix and entered a socket. Then decides to switch on nat mode and then enables haproxy
  2. user selected nat mode and haproxy protocol, now we need to only allow selecting type tcp, no udp or unix socket. This will be very confusing.
  3. etc.

@Kxiru Kxiru force-pushed the adjust-listen-and-connect-inputs branch 2 times, most recently from c4b35b2 to 4262f1c Compare September 18, 2024 16:20
@Kxiru Kxiru requested a review from edlerd September 18, 2024 16:20
edlerd
edlerd previously approved these changes Sep 18, 2024
Copy link
Collaborator

@edlerd edlerd left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This works so well now, good stuff 👍

QA and code LGTM, some very tiny nitpicks on the code, can also merge as is.

src/components/forms/ProxyDeviceForm.tsx Outdated Show resolved Hide resolved
src/components/forms/ProxyDeviceForm.tsx Outdated Show resolved Hide resolved
src/util/proxyDevices.tsx Outdated Show resolved Hide resolved
@Kxiru Kxiru force-pushed the adjust-listen-and-connect-inputs branch from 4262f1c to 49d5df9 Compare September 18, 2024 20:53
@Kxiru Kxiru requested a review from edlerd September 18, 2024 20:54
@Kxiru Kxiru merged commit c25e007 into canonical:main Sep 18, 2024
12 checks passed
github-actions bot pushed a commit that referenced this pull request Sep 18, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants